Skip to content

Fix module fixtures with --doctest-modules#14540

Open
minbang930 wants to merge 1 commit into
pytest-dev:mainfrom
minbang930:fix-14533-doctest-module-fixtures
Open

Fix module fixtures with --doctest-modules#14540
minbang930 wants to merge 1 commit into
pytest-dev:mainfrom
minbang930:fix-14533-doctest-module-fixtures

Conversation

@minbang930
Copy link
Copy Markdown

@minbang930 minbang930 commented Jun 1, 2026 •
edited
Loading

Summary

  • Fix fixture registration when --doctest-modules collects a Python module before the normal module collector.
  • Key parsed fixture holders by their visibility anchor, so normal tests get module-scoped FixtureDefs without relaxing node-based scoping.
  • Add a regression test, changelog entry, and AUTHORS entry.

Closes #14533.

Test plan

  • PYTHONPATH=src python /tmp/pytest-14533-repro/run_checkout_pytest.py -q testing/test_doctest.py::TestDoctests::test_module_fixture_available_to_normal_test_with_doctestmodules testing/test_doctest.py::TestDoctests::test_doctestmodule_with_fixtures testing/test_doctest.py::TestDoctests::test_doctestmodule_three_tests testing/test_doctest.py::TestDoctestAutoUseFixtures — 37 passed in 0.75s
  • PYTHONPATH=src python /tmp/pytest-14533-repro/run_checkout_pytest.py -q testing/test_conftest.py::test_conftest_fixture_scoping_with_testpaths_outside_rootdir testing/test_conftest.py::test_conftest_fixture_from_ancestor_above_rootdir testing/deprecated_test.py::TestFixtureNodeidDeprecations — 6 passed in 0.14s
  • PYTHONPATH=src python /tmp/pytest-14533-repro/run_checkout_pytest.py -q testing/python/fixtures.py::TestFixtureManagerParseFactories::test_parsefactories_conftest testing/python/fixtures.py::TestFixtureManagerParseFactories::test_parsefactories_conftest_and_module_and_class testing/python/fixtures.py::TestFixtureManagerParseFactories::test_parsefactories_relative_node_ids — 3 passed in 0.10s
  • git diff --check
  • python -m compileall -q src/_pytest/fixtures.py testing/test_doctest.py
  • python -m ruff check src/_pytest/fixtures.py testing/test_doctest.py
  • python -m ruff format --check src/_pytest/fixtures.py testing/test_doctest.py

AI assistance

OpenAI Codex helped reproduce the bug, draft the regression test, and prepare the initial patch. I reviewed the diff, checked the fixture scoping risk, added the changelog and AUTHORS entries, reran the targeted tests, and take responsibility for the change.

  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits.
  • Add closes #14533 to the PR description and commit message.
  • Credit the AI agent in a Co-authored-by commit trailer.
  • Create a changelog file in changelog/.
  • Add myself to AUTHORS in alphabetical order.

@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jun 1, 2026
RonnyPfannschmidt
RonnyPfannschmidt reviewed Jun 2, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good start and a fix

please expand the docs on the test

@bluetech a later major followup could make dedicated fs nodes part of the collection tree and specific nodes could decide if they ancor fixtures in the file node or the specific node

this first idea needs more iteration

Comment thread testing/test_doctest.py
reprec = pytester.inline_run(p, "--doctest-modules")
reprec.assertoutcome(passed=1)

def test_module_fixture_available_to_normal_test_with_doctestmodules(
Copy link
Copy Markdown
Member

@RonnyPfannschmidt RonnyPfannschmidt Jun 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a mark that links the issue and add a note on the regression on the docstring

@minbang930 minbang930 force-pushed the fix-14533-doctest-module-fixtures branch from eeed18e to 0ae2a23 Compare June 2, 2026 07:40
@minbang930
Copy link
Copy Markdown
Author

minbang930 commented Jun 2, 2026

Thanks, updated the regression test docs for the DoctestModule/Module fixture case. CI is green now.

@bluetech
Copy link
Copy Markdown
Member

bluetech commented Jun 2, 2026

If we key _holderobjseen by (obj, node), under which conditions will it be effective? I don't think parsefactories needs to be idempotent. So if this is the solution we go with, I prefer to just remove _holderobjseen.

The PR should document in the changelog the fact that higher-scoped autouse fixtures will now execute twice when using --doctest-modules, and how to avoid it if that's a problem (move said fixtures to conftest.py). We should also add a test for this.

If we impart to users the mental model that --doctest-modules effectively means that each python file is processed twice, I think the fixture behavior would make sense to them.

@bluetech a later major followup could make dedicated fs nodes part of the collection tree and specific nodes could decide if they ancor fixtures in the file node or the specific node

Interesting idea, but regarding the "specific nodes could decide if they ancor fixtures in the file node or the specific node" part:

  • If e.g. Module and DoctestModule decide differently, then fixtures will still be registered twice, so no good.
  • If they both choose to anchor at the FS node, it seems sort of ugly to me for the first one to "take over" the FS node and rely on a _holderobjseen-like mechanism to avoid duplication (ugly because it breaks encapsulation, which is why I'd really like to get rid of _holderobjseen).
  • So presumably the way would be for the FS node itself to do the parsefactories, and remove it from Module and DoctestModule. But parsefactories requires an obj which I think is not appropriate for a pure "FS" node to know about.

@minbang930 minbang930 force-pushed the fix-14533-doctest-module-fixtures branch from 0ae2a23 to c70b02a Compare June 2, 2026 10:59
@minbang930 @codex
Fix module fixtures with --doctest-modules
0526d9b 
pytest can collect the same Python module through DoctestModule and Module. FixtureManager skipped the second parse because it keyed parsed holders only by module object.

Key parsed holders by visibility anchor too. Normal tests get FixtureDefs anchored to their Module collector, and node-based fixture scoping remains unchanged.

Closes pytest-dev#14533.

Co-authored-by: OpenAI Codex <codex@openai.com>
@minbang930 minbang930 force-pushed the fix-14533-doctest-module-fixtures branch from c70b02a to 0526d9b Compare June 2, 2026 11:14
@minbang930
Copy link
Copy Markdown
Author

minbang930 commented Jun 2, 2026

Updated per feedback: removed _holderobjseen, documented the higher-scoped autouse fixture caveat, and added a regression test for it.

CI is green now.

@bluetech bluetech mentioned this pull request Jun 2, 2026
Closed
@RonnyPfannschmidt
Copy link
Copy Markdown
Member

RonnyPfannschmidt commented Jun 2, 2026

@bluetech hence the need for iteration - theres need for certain cooperation to make things work nice

technically i would actually go as far as considering doctestmodules a collector that intently is actually a child of the module, that it is at the same level as the module its "contained in" is the real issue for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

enabling doctest-modules causes fixture to not be found

3 participants

@minbang930 @bluetech @RonnyPfannschmidt